Replace GraphMap’s map with a moka::sync::Cache #1224

chirino · 2025-01-31T17:11:54Z

Fixes issue #1218

chirino · 2025-01-31T17:13:17Z

We should add some tests around the cache eviction edge cases:

Verify old sboms are dropped as the cache fills
Find out what happens when the cache is smaller a single request requires.

chirino · 2025-01-31T20:12:53Z

modules/analysis/src/model.rs

+    pub(crate) fn set_approximate_memory_size(&self) -> PackageNode {
+        // Is there a better way to do this?
+        let size = size_of::<PackageNode>()
+            + self.sbom_id.len()
+            + self.node_id.len()
+            + self.purl.iter().fold(0, |acc, purl|
+                // use the json string length as an approximation of the memory size
+                acc + serde_json::to_string(purl).unwrap_or_else(|_| "".to_string()).len())
+            + self.cpe.iter().fold(0, |acc, cpe|
+                // use the json string length as an approximation of the memory size
+                acc + serde_json::to_string(cpe).unwrap_or_else(|_| "".to_string()).len())
+            + self.name.len()
+            + self.version.len()
+            + self.published.len()
+            + self.document_id.len()
+            + self.product_name.len()
+            + self.product_version.len();
+
+        PackageNode {
+            approximate_memory_size: size.try_into().unwrap_or(u32::MAX),
+            ..self.clone()
+        }
+    }


Is there a better way to estimate how much memory this thing uses up?

Kinda assuming that saving the approximate_memory_size will give us savings later, it might not.

I switched to using the DeepSizeOf trait which has a derive macro and it seems to give much better results.

chirino · 2025-01-31T22:32:32Z

@JimFuller-RedHat I think I'm happy with this now. I'm going to keep it draft until #1217 goes in so you guys don't suffer the conflicts. But if you have idle time maybe worth a gander in case you think this went in the wrong direction.

JimFuller-RedHat · 2025-02-03T12:47:58Z

@chirino this looks really great (you are amazing!) ... and yes its likely to change a little bit after our PR (will review in full once we get that one into main) ... one thought was do we need to expose an env var to control params for easy configuration ?

chirino · 2025-02-03T17:07:52Z

added cli option to allow for configuration, squashed and rebased off main.

JimFuller-RedHat · 2025-02-03T18:38:57Z

modules/analysis/src/config.rs

+    #[arg(
+        id = "max-cache-size",
+        long,
+        env = "TRUSTD_MAX_CACHE_SIZE",


nit mind to add a line on https://github.com/trustification/trustify/blob/main/docs/env-vars.md ?

JimFuller-RedHat · 2025-02-03T18:42:01Z

Cargo.toml

 bytesize = "1.3"
 chrono = { version = "0.4.35", default-features = false }
 clap = "4"
+moka = "0.12.10"


have you consider also exposing moka expiration policies TTL and TTI as env vars ? I was hoping that we can control both time and mem dimensions ... Moka looks great!

Yeah was considering it. But at least as this PR is, we should be able to avoid blowing up the memory usage which was the main goal of this PR.

JimFuller-RedHat · 2025-02-03T18:44:59Z

modules/analysis/src/service/load.rs


-        self.graph.write().insert(distinct_sbom_id.to_string(), g);
+        let g = Arc::new(g);
+        self.graph_cache


it occurred to me there might be times when we want to suspend caching (might be useful for debugging for example)... not sure if this is important or not to consider for right now

JimFuller-RedHat · 2025-02-03T18:45:58Z

modules/analysis/src/service/mod.rs

 #[derive(Clone)]
 pub struct AnalysisService {
-    graph: Arc<RwLock<GraphMap>>,
+    graph_cache: Arc<GraphMap>,


I would wait till @ctron reviews ... previously he reworked things in this area and it was a bit tricky.

JimFuller-RedHat · 2025-02-03T18:49:27Z

modules/analysis/src/service/test.rs

+    let all_graphs = service.load_all_graphs(&ctx.db).await?;
+    assert_eq!(all_graphs.len(), 2);
+
+    let big_sbom_size = service.cache_size_used() - small_sbom_size;


very nice !

JimFuller-RedHat

apart from a few minor observations - looks great to me - as @ctron did the final fixes on the service invoke stuff good to get his eyeballs on it as well (as it was more then a little fiddly).

ctron

Looks good. A few things to iron out.

And I really would to have more nested use sections with every PR rather than less 😉

ctron · 2025-02-04T08:47:19Z

modules/analysis/src/config.rs

+        env = "TRUSTD_MAX_CACHE_SIZE",
+        default_value = "200 MB",
+        help = "Maximum size of the graph cache.  You can use binary units like 'kb', 'mb' or 'gb'.",
+        value_parser = u64_from_size


I'd like to keep this aligned with the existing way to parse byte sizes: https://github.com/trustification/trustify/blob/a7e2855368841cd6e667b35aab5608d411330749/common/infrastructure/src/app/http.rs#L134-L141

ctron · 2025-02-04T08:47:54Z

modules/analysis/src/model.rs

+    fmt,
+    ops::{Deref, DerefMut},
+};
+


Please merge and nest this.

ok. still wish tooling automated this.

ctron · 2025-02-04T08:52:08Z

modules/analysis/src/service/mod.rs

        &self,
        query: impl Into<GraphQuery<'a>> + Debug,
-        distinct_sbom_ids: Vec<String>,
+        graphs: Vec<(String, Arc<PackageGraph>)>,


It looks like it's not necessary to pass a Vec, so maybe use &[Arc<PackageGraph>] instead.

You right, will fix

ctron · 2025-02-04T08:52:41Z

modules/analysis/src/service/render.rs


 impl AnalysisService {
-    pub fn render_dot(&self, sbom: &str) -> Option<String> {
+    pub fn render_dot(&self, graph: Arc<PackageGraph>) -> Option<String> {


Is the Arc really required? Or is a &PackageGraph good enough?

ref is good enough.

ctron · 2025-02-04T08:53:33Z

server/src/profile/api.rs

        };

        Ok(InitData {
+            analysis: AnalysisService::new_sized(run.analysis.max_cache_size),


I think it would be great it one could construct an AnalysisService from an AnalysisConfig directly.

Will change that around. And give AnalysisConfig a default.

ctron · 2025-02-04T08:55:03Z

modules/analysis/src/service/mod.rs

    /// of having its own cache. So creating a new instance should be a deliberate choice.
    #[allow(clippy::new_without_default)]
    pub fn new() -> Self {
+        Self::new_sized(1024 * 1024 * 100)


I am worried this gets overlooked. It looks like we use this for tests only. One way could be to add #[cfg(test]. Or drop it an have a const default value for tests.

I'll just drop it and fix it in the tests.

ctron · 2025-02-04T15:14:36Z

modules/analysis/src/service/mod.rs

    {
        let query = query.into();

        // RwLock for reading hashmap<graph>


There's not rwlock anymore, is there?

ctron

I'd still prefer the use statements nested 😉

Fixes issue guacsec#1218 Signed-off-by: Hiram Chirino <[email protected]>

chirino force-pushed the lru branch from 97217ca to b1bea9d Compare January 31, 2025 17:15

chirino commented Jan 31, 2025

View reviewed changes

chirino force-pushed the lru branch from 2569af6 to b97ef93 Compare February 3, 2025 17:06

chirino marked this pull request as ready for review February 3, 2025 17:07

chirino requested a review from JimFuller-RedHat February 3, 2025 17:07

JimFuller-RedHat reviewed Feb 3, 2025

View reviewed changes

chirino requested a review from ctron February 3, 2025 18:47

JimFuller-RedHat reviewed Feb 3, 2025

View reviewed changes

JimFuller-RedHat approved these changes Feb 3, 2025

View reviewed changes

ctron requested changes Feb 4, 2025

View reviewed changes

ctron mentioned this pull request Feb 4, 2025

Expose metrics of analysis graph #1235

Closed

ctron reviewed Feb 4, 2025

View reviewed changes

ctron approved these changes Feb 4, 2025

View reviewed changes

chirino force-pushed the lru branch from 1b27912 to 191a7aa Compare February 4, 2025 15:33

Replace GraphMap’s map with a moka::sync::Cache

0b70b91

Fixes issue guacsec#1218 Signed-off-by: Hiram Chirino <[email protected]>

chirino force-pushed the lru branch from 191a7aa to 0b70b91 Compare February 4, 2025 15:38

chirino added this pull request to the merge queue Feb 4, 2025

Merged via the queue into guacsec:main with commit 0b70b91 Feb 4, 2025
3 checks passed

chirino deleted the lru branch February 4, 2025 16:46

Replace GraphMap’s map with a moka::sync::Cache #1224

Replace GraphMap’s map with a moka::sync::Cache #1224

Uh oh!

Conversation

chirino commented Jan 31, 2025

Uh oh!

chirino commented Jan 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chirino Feb 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chirino commented Jan 31, 2025

Uh oh!

JimFuller-RedHat commented Feb 3, 2025

Uh oh!

chirino commented Feb 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JimFuller-RedHat left a comment

Choose a reason for hiding this comment

Uh oh!

ctron left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctron Feb 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ctron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

chirino Feb 2, 2025 •

edited

Loading

ctron Feb 4, 2025 •

edited

Loading